Skip to content

enable automatic type convert #708

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

enable automatic type convert #708

wants to merge 1 commit into from

Conversation

@mck
Copy link
Collaborator

@mck mck commented May 25, 2025 •
edited by github-actions bot
Loading

PR Type

Enhancement, Tests

Description

Auto-insert converter on convertible type mismatch
Add TypeConverterNode UI and engine node
Extend schema conversion and registry APIs
Support design token conversions with tests

Changes walkthrough 📝

Relevant files
Enhancement
8 files
typeConverterNode.tsx
Add TypeConverterNode React component                                       
+89/-0   
connect.ts
Insert converter between incompatible nodes                           
+107/-1 
graph.tsx
Register TYPE_CONVERTER node type                                               
+12/-3   
index.ts
Include typeConverter in generic nodes                                     
+3/-1     
typeConverter.ts
Implement TypeConverter graph engine node                               
+72/-0   
index.ts
Extend schema conversion logic                                                     
+364/-4 
typeConversions.ts
Add design token conversion extensions                                     
+122/-0 
conversions.ts
Add design token schema conversions                                           
+101/-0 
Configuration changes
2 files
ids.ts
Define TYPE_CONVERTER node identifier                                       
+1/-0     
index.ts
Auto-register token conversion extensions                               
+3/-0     
Tests
5 files
connectionWithConversion.test.ts
Add conversion connection logic tests                                       
+125/-0 
typeConverter.test.ts
Add TypeConverter node tests                                                         
+39/-0   
typeConversions.test.ts
Test design token conversion extensions                                   
+79/-0   
automaticConversion.test.ts
Integration tests for automatic conversion                             
+78/-0   
endToEndConversion.test.ts
E2E tests for token conversion workflow                                   
+180/-0 
Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
    •

    @changeset-bot
    Copy link

    changeset-bot bot commented May 25, 2025

    ⚠️ No Changeset found

    Latest commit: 7aab6ff

    Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

    This PR includes no changesets

    When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

    Click here to learn what changesets are, and how to add one.

    Click here if you're a maintainer who wants to add a changeset to this PR

    @github-actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Missing Fallback

    The automatic insertion logic for the TypeConverterNode silently skips conversion if nodeLookup[TYPE_CONVERTER] is undefined. Ensure there is a clear fallback or warning to prevent silent failures during node connections.

     
    if (needsConversion) {
  // Create a type converter node
  const TypeConverterClass = nodeLookup[TYPE_CONVERTER];
  if (TypeConverterClass) {
    const converterNode = new TypeConverterClass({ graph });
    ReactFlow API Use

    The code calls reactFlowInstance?.addNodes(converterFlowNode), but React Flow's API may expect a different method signature (e.g., addNode or an array). Verify that this matches the version in use to avoid UI integration errors.

     
    reactFlowInstance?.addNodes(converterFlowNode);
    Broad ANY Conversion

    The update to canConvertSchemaTypes now treats schemas with src.$id === ANY as convertible to any target. Confirm this doesn't introduce unintended conversions by reviewing downstream impacts.

     
    if (target.$id === ANY) return true;
if (src.$id === ANY) return true;

    @github-actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Use array for addNodes call

    Use React Flow’s addNodes API correctly by passing an array of nodes, ensuring the
    converter node is actually inserted into the view.

    packages/graph-editor/src/editor/actions/connect.ts [114]

    -reactFlowInstance?.addNodes(converterFlowNode);
+reactFlowInstance?.addNodes([converterFlowNode]);
    Suggestion importance[1-10]: 7

    __

    Why: Passing a single node object to addNodes may not insert it properly when the API expects an array of nodes, so wrapping converterFlowNode in an array fixes a potential insertion bug.

    		Medium
    Guard connection parameters

    Add a guard at the start of the conversion block to verify that params.source,
    params.sourceHandle, params.target, and params.targetHandle are defined before using
    them, preventing possible runtime undefined errors.

    packages/graph-editor/src/editor/actions/connect.ts [133-140]

    +if (!params.source || !params.sourceHandle || !params.target || !params.targetHandle) {
+  return;
+}
 const sourceToConverterFlowEdge = {
   id: sourceToConverterEdge.id,
-  source: params.source!,
-  sourceHandle: params.sourceHandle!,
+  source: params.source,
+  sourceHandle: params.sourceHandle,
   target: converterNode.id,
   targetHandle: 'value',
   type: 'custom',
 };
    Suggestion importance[1-10]: 5

    __

    Why: Adding a guard for params.source and related handles prevents runtime undefined errors, but it's a minor defensive check and can be addressed elsewhere in the call chain.

    		Low
    General
    Simplify edge filtering

    Simplify the edge filtering by using filter and array spread, which improves
    readability and avoids manual accumulator mutations.

    packages/graph-editor/src/editor/actions/connect.ts [159-175]

     return setEdges((eds) => {
-  const newEdgs = eds.reduce(
-    (acc, edge) => {
-      if (
-        edge.targetHandle == params.targetHandle &&
-        edge.target === params.target
-      ) {
-        return acc;
-      }
-      acc.push(edge);
-      return acc;
-    },
-    [sourceToConverterFlowEdge, converterToTargetFlowEdge] as Edge[],
+  const filtered = eds.filter(
+    edge =>
+      !(edge.targetHandle === params.targetHandle && edge.target === params.target)
   );
-  return newEdgs;
+  return [sourceToConverterFlowEdge, converterToTargetFlowEdge, ...filtered];
 });
    Suggestion importance[1-10]: 6

    __

    Why: Replacing the reduce logic with a simple filter and array spread improves readability and maintains the same functionality for edge cleanup.

    		Low

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    Review effort 4/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @mck